Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP: Amino non len prefixed default #2234

Closed
wants to merge 6 commits into from
Closed

WIP: Amino non len prefixed default #2234

wants to merge 6 commits into from

Conversation

liamsi
Copy link
Contributor

@liamsi liamsi commented Sep 4, 2018

These are the changes that will be necessary as soon as tendermint/go-amino#222 lands in a release and the sdk wants to catch up to the latest amino release.
(See discussion here: tendermint/go-amino#219)

I'll update this PR accordingly.

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added entries in PENDING.md with issue #

  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@liamsi
changes to catch up when this will be released and the sdk updates
f252879 
tendermint/go-amino#222
@liamsi
Merge remote-tracking branch 'remotes/origin/develop' into amino_non_…
ad4e872 
…len_prefixed_default

# Conflicts:
#	baseapp/baseapp.go
#	baseapp/baseapp_test.go
#	client/context/helpers.go
#	client/tx/query.go
#	types/rational_test.go
#	x/auth/client/cli/account.go
#	x/gov/client/cli/tx.go
#	x/gov/client/rest/rest.go
#	x/gov/handler.go
#	x/slashing/client/rest/query.go
#	x/stake/keeper/validator.go
@ValarDragon
Copy link
Contributor

I feel like almost all of these don't need to be length prefixed. (I didn't look super deeply into it, but I think all of them don't need to be length prefixed?)

Also I think tendermint will need a PR to upgrade it as well, since tendermint will share the same dependency due to dep overrides.

@liamsi liamsi force-pushed the amino_non_len_prefixed_default branch from ad4e872 to 011f9a1 Compare September 8, 2018 17:06
@liamsi liamsi requested a review from zramsay as a code owner September 8, 2018 17:06
@liamsi
Copy link
Contributor Author

liamsi commented Sep 8, 2018 •
edited
Loading

I agree @ValarDragon! Most cases won't need length encoding.

I think it is good to make this more explicit that people scratch their head first to see if they actually need length prefixed encoding or not. I discussed with Jae, and we will upgrade in two steps. See comment inhttps://github.com/tendermint/go-amino/pull/222
I've updated the code accordingly (only rename MarshalBinary)

@liamsi
Copy link
Contributor Author

liamsi commented Sep 8, 2018

And I'll create a PR out of https://github.com/tendermint/tendermint/compare/develop...amino_explicit_len_prefix?expand=1
as soon as this is merged (in amino).

@ValarDragon
Copy link
Contributor

ValarDragon commented Sep 8, 2018 •
edited
Loading

Sounds like a good idea. Do you think we should convert things from length prefixed to not length prefixed in this PR or a separate PR? If its a separate PR, this seems good to merge imo, as soon as we update tendermint.

Looking through this, the only place where length prefixing may be necessary (imo) is iavlstore stuff.

@liamsi
Copy link
Contributor Author

liamsi commented Sep 8, 2018

Do you think we should convert things from length prefixed to not length prefixed in this PR or a separate PR?

Yeah, I think this should be done in a separate PR after this gets merged. I wouldn't be surprised if length prefixing isn't necessary in most places though.

@cwgoes
Copy link
Contributor

cwgoes commented Sep 27, 2018

@liamsi What's the status on this - looks like the upstream change is still pending, is that blocked on anything?

@liamsi
WIP: Merge remote-tracking branch 'remotes/origin/develop' into amino…
d306e8f 
…_non_len_prefixed_default

# Conflicts:
#	baseapp/baseapp.go
#	baseapp/baseapp_test.go
#	examples/democoin/x/oracle/keeper_keys.go
#	examples/democoin/x/oracle/oracle_test.go
#	store/rootmultistore.go
#	types/lib/linear.go
#	types/lib/linear_test.go
#	x/auth/account_test.go
#	x/auth/mapper.go
#	x/gov/handler.go
#	x/ibc/mapper.go
#	x/params/keeper.go
#	x/params/keeper_test.go
#	x/slashing/signing_info.go
#	x/stake/keeper/keeper.go
#	x/stake/keeper/validator.go
@liamsi
Copy link
Contributor Author

liamsi commented Oct 22, 2018 •
edited by alexanderbez
Loading

Still waiting for approval on tendermint/go-amino#233
@ebuchman created a pre-release here: https://github.com/tendermint/go-amino/releases/tag/v0.13.0-rc0
but it doesn't contain the commit which does the renaming (marshalbinary -> marshalbinarylengthprefixed).

I think it makes sense to:

  1. create a proper amino release
  2. use it in tendermint (and update tm to the names)
  3. update and merge this PR.

@liamsi liamsi force-pushed the amino_non_len_prefixed_default branch from 011f9a1 to d306e8f Compare October 22, 2018 12:53
@liamsi
Copy link
Contributor Author

liamsi commented Oct 23, 2018

This can be merged as is (assuming there are no new merge conflicts) as soon as:
tendermint/tendermint#2687 and tendermint/tendermint#2690 are merged

@cwgoes
Copy link
Contributor

cwgoes commented Oct 24, 2018

Sadly there are now new merge conflicts. Let's include this with SDK release 0.26 (just blocked on Tendermint 0.26).

@jackzampolin jackzampolin changed the title [WIP]: Amino non len prefixed default WIP: Amino non len prefixed default Oct 25, 2018
@liamsi
Bring back (Un)marshalBinaryBare:
96bd955 
- initially this was also renamed but we decided to do this in an
incremental, two step approach

(see discussion: tendermint/go-amino#222)
@liamsi liamsi force-pushed the amino_non_len_prefixed_default branch from cf656b5 to 5fb1be1 Compare October 29, 2018 15:46
@liamsi
Copy link
Contributor Author

liamsi commented Oct 29, 2018

Resolved the merge conflicts. Additionally, tendermint 0.26 will use amino 0.14.0, which (additionally to the renaming) encodes ints differently (varint / non-zigzag). Can test it with the sdk as soon as tm is released.
ref tendermint/tendermint#2710

@liamsi liamsi force-pushed the amino_non_len_prefixed_default branch from 5fb1be1 to ea7355a Compare October 29, 2018 15:54
@liamsi liamsi force-pushed the amino_non_len_prefixed_default branch from c6add6c to 7b5467d Compare October 29, 2018 16:15
@liamsi
Copy link
Contributor Author

liamsi commented Oct 31, 2018 •
edited
Loading

Looks like the changes made in this PR are also contained in #2633. When #2633 gets merged, we can close this one.

@jaekwon
Copy link
Contributor

jaekwon commented Nov 3, 2018

oops, I didn't see this. thank you ismail.

@cwgoes
Copy link
Contributor

cwgoes commented Nov 5, 2018

Closing as the Tendermint 0.26 PR has been merged.

@cwgoes cwgoes closed this Nov 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ValarDragon ValarDragon ValarDragon approved these changes

@cwgoes cwgoes Awaiting requested review from cwgoes

@ebuchman ebuchman Awaiting requested review from ebuchman

@rigelrozanski rigelrozanski Awaiting requested review from rigelrozanski

@zramsay zramsay Awaiting requested review from zramsay

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@liamsi @ValarDragon @cwgoes @jaekwon @rigelrozanski